Add VALID_MODES constant and validate tracker mode at initialization#186
Add VALID_MODES constant and validate tracker mode at initialization#186JuanVqz wants to merge 5 commits into
Conversation
Centralizes valid tracker modes so adding/removing an option requires a single change. Raises ArgumentError early instead of silently no-oping on invalid input, covering both library and CLI entry points.
Extract constants to valid_modes.rb so exe/deprecations only loads the full lib when needed. Use VALID_MODES_DISPLAY to avoid repeated map/join. Loosen spec assertions to not couple on mode count or message wording.
etagwerker
left a comment
etagwerker
left a comment
There was a problem hiding this comment.
@JuanVqz I think this might introduced an unwanted side effect (?)
bin/deprecations run without --tracker-mode is probably broken. The CLI builds:
command = "DEPRECATION_TRACKER=#{tracker_mode} #{rspec_command} ..."
When tracker_mode is nil, the spawned child sees ENV["DEPRECATION_TRACKER"] = ""
Empty string is truthy in Ruby, so:
mode = opts[:mode] || ENV["DEPRECATION_TRACKER"] || :save # => ""
@mode = mode ? mode.to_sym : :save # => :""
# new check raises ArgumentError
Before this PR that silently no-op'd in after_run. After this PR it crashes at construction.
- Treat a blank mode (from opts[:mode] or DEPRECATION_TRACKER) as unset and default to :save instead of raising, so the documented `mode: ENV["DEPRECATION_TRACKER"]` wiring no longer crashes on an empty value. Add DeprecationTracker.sanitize_mode for this. - Add DeprecationTracker.valid_mode? as the single source of truth for mode validation, used by both the constructor and the deprecations CLI. - Replace VALID_MODES_DISPLAY constant with a memoized .valid_modes_display class method. - bin/deprecations run now aborts with a clear message when --tracker-mode is missing or invalid. - Specs: assert the ArgumentError message names the bad value, cover blank env/opts defaulting, and add .valid_mode?/.valid_modes_display blocks.
Address review on PR #186: - Reformat invalid-mode ArgumentError to 'one of: save, compare. Got: ...' for clearer reading; update matching spec regexes. - Remove tautological VALID_MODES 'contains save and compare' test.
|
@arielj addressed latest code changes 👍 |
| tracker_mode = opts[:tracker_mode] | ||
| unless DeprecationTracker.valid_mode?(tracker_mode) | ||
| abort "Invalid --tracker-mode #{tracker_mode.inspect}. Must be one of: #{DeprecationTracker.valid_modes_display}." | ||
| end |
There was a problem hiding this comment.
Should this guard skip the case where no --tracker-mode is passed? --tracker-mode is optional, so on the documented default deprecations run (README + the help examples) opts[:tracker_mode] is nil, and valid_mode?(nil) is falsey, so this aborts with Invalid --tracker-mode nil. before reaching rspec.
I cloned the branch and confirmed it: deprecations run (no --tracker-mode) exits 1 on this branch, whereas on main it falls through to DEPRECATION_TRACKER= bundle exec rspec ... and the library treats the blank value as the default save. That blank-as-default path is exactly what sanitize_mode adds for the library, but the CLI guard rejects nil/blank before that logic runs.
What about only validating when a mode was actually supplied, so a genuine typo (--tracker-mode bogus) still fails loudly but the default keeps working?
| tracker_mode = opts[:tracker_mode] | |
| unless DeprecationTracker.valid_mode?(tracker_mode) | |
| abort "Invalid --tracker-mode #{tracker_mode.inspect}. Must be one of: #{DeprecationTracker.valid_modes_display}." | |
| end | |
| tracker_mode = DeprecationTracker.sanitize_mode(opts[:tracker_mode]) | |
| if tracker_mode && !DeprecationTracker.valid_mode?(tracker_mode) | |
| abort "Invalid --tracker-mode #{tracker_mode.inspect}. Must be one of: #{DeprecationTracker.valid_modes_display}." | |
| end |
3 participants
Description
Introduces
DeprecationTracker::VALID_MODESas a single source of truth for accepted tracker modes (save,compare). Constants live inlib/deprecation_tracker/valid_modes.rbso theexe/deprecationsCLI can load them without pulling in the full library.Motivation and Context
Valid mode values were duplicated as inline literals across
lib/deprecation_tracker.rbandexe/deprecations. Adding or removing a mode required updating every literal. WithVALID_MODES, one change propagates to all validation, error messages, and CLI help text automatically.Additionally, passing an invalid mode previously silently no-oped (neither
savenorcompareran after the test suite). It now raisesArgumentErrorearly, covering both the library and CLI entry points.How Has This Been Tested?
ArgumentErrorENV['DEPRECATION_TRACKER']value viainit_trackerVALID_MODESdescribe block: constant membership and all valid modes construct without errorbundle exec rspec spec/deprecation_tracker_spec.rb)Screenshots:
N/A
I will abide by the code of conduct